Various fixes, part 3 by quietbits · Pull Request #961 · stellar/js-stellar-base

quietbits · 2026-03-31T21:44:20Z

Addresses items 1-9 for js-stellar-base in this doc.

quietbits · 2026-03-31T21:45:19Z

  signer: Keypair | SigningCallback,
  validUntilLedgerSeq: number,
-  networkPassphrase: string = Networks.TESTNET,
+  networkPassphrase: string,


This is a breaking change, but I think we should make it. This is the only place where we silently set the network passphrase to testnet. Everywhere else it is required.

github-actions · 2026-03-31T21:45:34Z

Size Change: +5.14 kB (+0.29%)

Total Size: 1.76 MB

📦 View Changed

Filename	Size	Change
`dist/stellar-base.cjs.js`	696 kB	+2.02 kB (+0.29%)
`dist/stellar-base.js`	715 kB	+2.09 kB (+0.29%)
`dist/stellar-base.min.js`	344 kB	+1.04 kB (+0.3%)

_{compressed-size-action}

Copilot

Pull request overview

This PR bundles multiple correctness and safety fixes across rational approximation utilities, transaction immutability guarantees, Soroban SCVal conversion hardening, muxed account ID validation, and Soroban auth signing.

Changes:

Improve best_r continued-fraction approximation to recover near int32 boundaries (instead of throwing).
Add optional immutableTx support so TransactionBase.tx can return a defensive XDR copy to prevent external mutation.
Harden nativeToScVal against prototype-pollution edge cases and add regression tests; validate muxed-account uint64 IDs for overflow.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`src/util/continued_fraction.ts`	Adds semi-convergent recovery when convergents degenerate near int32 bounds.
`test/unit/util/continued_fraction.test.ts`	Updates expectations and adds boundary/regression coverage for `best_r`.
`src/transaction_base.ts`	Implements defensive-copy behavior for `tx` getter behind `immutableTx`.
`src/transaction.ts`	Threads `immutableTx` option into `TransactionBase`.
`src/fee_bump_transaction.ts`	Threads `immutableTx` option into `TransactionBase` and inner transaction construction.
`src/transaction_builder.ts`	Adds `immutableTx` option and propagates it when building/fromXDR.
`test/unit/transaction.test.ts`	Adds tests asserting `tx` getter immutability behavior.
`src/scval.ts`	Uses prototype checks and `Object.hasOwn` to avoid prototype key pitfalls in map type hints.
`test/unit/scval.test.ts`	Adds prototype-pollution regression tests and cleans up some unsafe casts/formatting.
`src/muxed_account.ts`	Adds uint64-range validation for muxed account IDs (constructor + `setId`).
`test/unit/muxed_account.test.ts`	Adds overflow and max-uint64 acceptance tests.
`src/auth.ts`	Updates Soroban auth signing APIs around network passphrase handling.
`test/unit/auth.test.ts`	Updates auth tests to pass explicit network passphrase and adds network-differentiation test.
`test/unit/operation.test.ts`	Adds `toXDRPrice` zero-denominator regression test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T21:52:08Z

 export function authorizeInvocation(
  signer: Keypair | SigningCallback,
  validUntilLedgerSeq: number,
  invocation: xdr.SorobanAuthorizedInvocation,
  publicKey: string = "",
-  networkPassphrase: string = Networks.TESTNET,
+  networkPassphrase: string,
 ): Promise<xdr.SorobanAuthorizationEntry> {


authorizeInvocation declares publicKey with a default value (making it optional) but then has a required networkPassphrase parameter after it. TypeScript does not allow required parameters to follow optional ones, so this signature should be reordered and/or networkPassphrase should also have a default (or be moved into an options object).

We'll keep the current order to make the breaking change "lighter". We'll add a note in JSDoc about why we have this order.

I think we would need to change the order right since the previous param is optional / has a default

It might make sense to just make publicKey required as well. Anyone seriously using this has to since the default passphrase was future net.

If we make publicKey required, it will be a breaking change and everyone who is relying on signer to be used as publicKey will be affected. There is no real benefit making it required, I think. It might cause more headache than good.

If we change the order of the arguments, everyone will need to update now (swap public key and network). Since both are strings, it will be easy to miss. If we do update, I'd rather pass an object instead. It's a bigger change, but it would be clear what needs to be updated. Thoughs, @Ryang-21 ?

We would already be making a breaking change since the default for the passphrase is removed. Users now need to do something like

authorizeInvocation( signer: Keypair.random(), validUntilLedgerSeq: 123, invocation: {}, publicKey: undefined, networkPassphrase: Networks.Testnet, )

So if they need to pass undefined anyways we might as well make it required?

We'll go with the object approach for args, which feels like the best option considering everything. 🤞

…ms object

quietbits added 7 commits March 31, 2026 14:05

util/continued_fraction updated

00b4cfc

muxed_account updated

552f6eb

Added operation test

ad22db8

Add immutable tx flag

06d451d

auth updated

6904d28

scval updated

eb6ce90

Added source acc test for getClaimableBalanceId()

4df96f1

quietbits commented Mar 31, 2026

View reviewed changes

Comment thread src/transaction_base.ts Outdated

quietbits requested a review from Copilot March 31, 2026 21:47

Copilot started reviewing on behalf of quietbits March 31, 2026 21:47 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

Copilot feedback

0d3750e

quietbits requested a review from Ryang-21 March 31, 2026 22:02

Ryang-21 approved these changes Apr 2, 2026

View reviewed changes

quietbits added 4 commits April 2, 2026 09:21

Always return immutable tx, remove opt-in immutableTx flag

b2fca1d

Breaking: Change authorizeInvocation() from positional args to a para…

dab2285

…ms object

Updated changelog

64e014e

Resolved merge conflicts

555c112

Ryang-21 reviewed Apr 2, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Ryang-21 reviewed Apr 2, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Ryang-21 reviewed Apr 2, 2026

View reviewed changes

Comment thread CHANGELOG.md Outdated

Clean up changelog

c4e1bf4

quietbits merged commit 1787a9e into typescript-migration Apr 2, 2026
7 checks passed

quietbits deleted the some-fixes-3 branch April 2, 2026 19:59

Uh oh!

Conversation

quietbits commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

quietbits Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

quietbits Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

Ryang-21 Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

Ryang-21 Apr 1, 2026

Choose a reason for hiding this comment

Uh oh!

quietbits Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Ryang-21 Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

quietbits Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

quietbits commented Mar 31, 2026 •

edited

Loading

github-actions Bot commented Mar 31, 2026 •

edited

Loading